-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add abstract functions for PoE enabling #2636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start - but (and don't take my word for it), I would assume that not every port on a device is necessarily capable of power delivery, so the API might need a way to signal PoE-eligibility for individual ports.
When it comes to the power state tuple-thingy, I would suggest using a NamedTuple
, which would greatly clarify what this magic tuple-thingy is.
let each implementation handle conversion if the actual state is represented as integers or whatever, but for this interface just let it be strings, e.g. DISABLED, AUTO or something
Codecov Report
@@ Coverage Diff @@
## master #2636 +/- ##
==========================================
+ Coverage 54.20% 54.74% +0.54%
==========================================
Files 558 560 +2
Lines 40634 40743 +109
==========================================
+ Hits 22026 22306 +280
+ Misses 18608 18437 -171
... and 26 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
the state options should be the same on the netbox for ports that support it, so no need to check that per interface. what we actually care about is if the interface supports poe, so this does that more explicitly
Made it a bit less magical. Instead of actual state and name, just return the names as a tuple of strings and let each management implementation handle conversion behind the scenes. Added function for checking if specific interface supports PoE |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. When we have two subclasses we'll know if it is good enough. We will then also have examples (and tests!)
@lunkwill42 needs to approve it, though.
python/nav/portadmin/handlers.py
Outdated
|
||
def get_poe_state(self, interface: manage.Interface) -> PoeState: | ||
"""Returns current PoE state of this interface""" | ||
raise NotImplementedError | ||
|
||
def interface_supports_poe(self, interface: manage.Interface) -> bool: | ||
"""Returns True if this interface supports PoE""" | ||
raise NotImplementedError | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When loading portadmin, youre gonna want to call get_poe_state for all poe enabled ports. Doing it one by one is not very efficient. Perhaps adding another function for getting ALL poe states would make sense. If this function returns poe state for all interfaces that support poe, then this response can be used to find out which interfaces do not support poe, making the interface_supports_poe function largely irrelevant.
Keeping the smaller functions for getting information about a specific interface would not hurt, but having functions that do the heavy lifting more efficient would definitely be useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, some methods already have signatures that allow for fetching interface information from a range of interfaces, like this:
nav/python/nav/portadmin/handlers.py
Lines 59 to 72 in 5d97c40
def get_interfaces( | |
self, interfaces: Sequence[manage.Interface] = None | |
) -> List[Dict[str, Any]]: | |
"""Retrieves running configuration switch ports on the device. | |
:param interfaces: Optional list of interfaces to filter for, as fetching | |
data for all interfaces may be a waste of time if only a | |
single interface is needed. The implementing | |
handler/protocol may not support this filter, so do not rely | |
on it. | |
:returns: A list of dicts with members `name`, `description`, `oper`, `admin` | |
and `vlan` (the latter being the access/untagged/native vlan ID. | |
""" | |
raise NotImplementedError |
I think that could be a useful pattern to re-use. In these methods, the sequence of interfaces is an optional argument. If omitted, the method will operate on all known interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems quite useful, I'll prob add something like this
python/nav/portadmin/handlers.py
Outdated
def get_poe_state_options(self) -> Tuple[PoeState, ...]: | ||
"""Returns the available options for enabling/disabling PoE on this netbox""" | ||
raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant to be called by the frontend to populate a dropdown menu or something. the user can then select one of the PoeState options, and this would then be fed into set_poe_state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the return value very specifically need to be a tuple? Usually I prefer annotating things as Sequence
unless there is a expectation of being able to use the result value specifically as a tuple or list object.
There is the same thing with dict
vs. Mapping
, but I must admit I haven't been using the latter as much as I probably should have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tuple was used to signify that this is not something that is meant to be manipulated in any way, since it just informs the static choices. Havent used Sequence myself but I can try it out here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a lot better without too much magic - but please do consider how bulk operations will work (preferably similar to other methods on the ManagementHandler
)
python/nav/portadmin/handlers.py
Outdated
def get_poe_state_options(self) -> Tuple[PoeState, ...]: | ||
"""Returns the available options for enabling/disabling PoE on this netbox""" | ||
raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the return value very specifically need to be a tuple? Usually I prefer annotating things as Sequence
unless there is a expectation of being able to use the result value specifically as a tuple or list object.
There is the same thing with dict
vs. Mapping
, but I must admit I haven't been using the latter as much as I probably should have.
I believe |
new `get_poe_states` supports single interface queries
python/nav/portadmin/handlers.py
Outdated
def interface_supports_poe(self, interface: manage.Interface) -> bool: | ||
"""Returns True if this interface supports PoE""" | ||
raise NotImplementedError | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I question the need for this function. When ive tried to implement this for cisco and juniper, it boils down to trying to get the state from the device and returning true/false based on if it found anything. It doesnt actually save any time over just calling get_poe_states
for the interface youre interested in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. You could simply change the definition of get_poe_state
to return something like Dict[int, Optional[PoeState]]
, where the value for a non-supported interface would just be None
.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks just fine as it is, but I also agree that the interface_supports_poe
might be redundant. Either that, or it could have a base implementation that calls get_poe_state_options()
for the interface and returns True/False based on whether the interface has any state options.
python/nav/portadmin/handlers.py
Outdated
def interface_supports_poe(self, interface: manage.Interface) -> bool: | ||
"""Returns True if this interface supports PoE""" | ||
raise NotImplementedError | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. You could simply change the definition of get_poe_state
to return something like Dict[int, Optional[PoeState]]
, where the value for a non-supported interface would just be None
.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
For #1938.
Adds empty functions to ManagementHandler that should be implemented by vendor specific handlers in different ways. Different
vendors support different PoE states, so
get_poe_state_options
should return all available options. These options should then be displayed in PortAdmin and be selectable.